fix(adapters): api_url custom-endpoint config (Slack/Discord/GitHub/Linear) + GitHub public get_installation_id#150
Conversation
…inear) + GitHub public get_installation_id Pre-existing parity gaps ported faithfully from upstream chat@4.30.0. (A) api_url custom-endpoint config (upstream 4.27.0, 6b17c60) — every high-level adapter reads `config.apiUrl ?? <ADAPTER>_API_URL` env and routes it into the underlying API client. Add the field (+ env fallback, default unchanged when unset) to the four adapters that were missing it: - Slack (index.ts:617,576-577,620-621): new `api_url` (+ SLACK_API_URL). Passed as `base_url` to BOTH the async AsyncWebClient cache and the synchronous web_client escape hatch. Omitted entirely when unset (slack_sdk rejects base_url=None and keeps its built-in default). - Discord (types.ts:18-19, index.ts:142): new `api_url` (+ DISCORD_API_URL) replacing the hardcoded DISCORD_API_BASE in `_discord_fetch` — the single funnel all Discord HTTP goes through. - GitHub (index.ts:201,217): new `api_url` (+ GITHUB_API_URL) for GitHub Enterprise Server. Threaded into `_github_api_request` and the installation-token exchange URL (the two hardcoded api.github.com sites). - Linear (index.ts:221,239,249, types.ts:51): new `api_url` (+ LINEAR_API_URL) overriding the GraphQL endpoint in `_graphql_query`. (B) GitHub public get_installation_id (index.ts:458-480) — add the public `get_installation_id(thread|str)` returning the fixed id (single-tenant app), the cached repo installation (multi-tenant), or None (PAT mode); raises when multi-tenant and uninitialized. Keeps the private `_get_installation_id(owner, repo)` helper it delegates to. Tests: tests/test_adapter_api_url_config.py (31) — per-adapter custom-url used / default-unchanged / env-fallback / config-wins-over-env, plus the six get_installation_id auth-mode branches. The usage-site assertions were verified to fail against the pre-fix sources.
|
Warning Review limit reached
More reviews will be available in 53 minutes and 9 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds custom api_url configuration options and environment variable fallbacks across the Discord, GitHub, Linear, and Slack adapters to support custom API base URLs. It also implements a public get_installation_id method on the GitHub adapter and includes comprehensive tests. One issue was identified in the GitHub adapter where a truthiness check on api_url_raw fails to honor an explicit empty string, and should be replaced with an is not None check.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # explicit empty string (CLAUDE.md truthiness-trap hazard). | ||
| config_api_url = config.get("api_url") | ||
| api_url_raw = config_api_url if config_api_url is not None else os.environ.get("GITHUB_API_URL") | ||
| self._api_url = api_url_raw.rstrip("/") if api_url_raw else GITHUB_API_BASE_URL |
There was a problem hiding this comment.
The truthiness check if api_url_raw will evaluate to False if api_url_raw is an explicit empty string (""), causing it to incorrectly fall back to GITHUB_API_BASE_URL. This violates the general rule of using is not None for optional values that can be falsy but valid, and contradicts the comment's stated intent to honor explicit empty strings.
Please use is not None instead of a truthiness check.
| self._api_url = api_url_raw.rstrip("/") if api_url_raw else GITHUB_API_BASE_URL | |
| self._api_url = api_url_raw.rstrip("/") if api_url_raw is not None else GITHUB_API_BASE_URL |
References
- When checking for optional values that can be falsy but valid (e.g., 0, empty string, empty list), use
is not Noneinstead of a truthiness check to avoid silently ignoring them.
…efault endpoint
The api_url config test module guarded every test behind a module-level
pytest.importorskip("slack_sdk"). Since slack_sdk lives only in the optional
slack/all extras (not the dev group CI installs), the whole file collected as
a single skip -- silently disabling the Discord/GitHub/Linear api_url tests and
the get_installation_id regression tests in CI (falsely green).
Stub slack_sdk into sys.modules (mirroring tests/test_slack_client_cache.py) so
the module imports without the real package and the Slack tests run too. The
file now reports 34 passed, 1 skipped (the lone skip is the genuine-sdk
end-to-end test, intentionally skipped when stubbed) instead of 1 skipped.
Empty-string api_url now falls back to the default endpoint, matching upstream's
truthy spread ...(this.apiUrl ? {...} : {}):
- Slack: empty apiUrl/env resolves to None, so base_url is omitted and slack_sdk
keeps https://slack.com/api/ (never passes base_url="").
- Linear: empty resolves to LINEAR_API_URL instead of POSTing to an empty URL.
- GitHub: empty resolves to https://api.github.com; fixed the misleading inline
comment that claimed the is-not-None check honored empty string.
- Discord: empty resolves to DISCORD_API_BASE instead of a relative request URL.
Added a per-adapter empty-string regression test (Slack/Discord/GitHub/Linear).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
CHANGELOG: add 'Pre-existing parity gaps closed (4.30 audit)' subsection covering the 7 ported fixes (PRs #147-#150) and a documented-exceptions note (Linear agent-sessions #151 deferred to 4.31 / #152; adapter-web and the GitHub/Linear native-client + message.subject halves stay Known Non-Parity). UPSTREAM_SYNC: - file-mapping table: add state-ioredis -> redis.py (IoRedisStateAdapter) and state-pg -> postgres.py rows (both ported + tested, were missing). - Known Non-Parity (platform gaps): add Linear agent-sessions row (#151) and an adapter-web row that splits the portable server-side WebAdapter (deferred) from the genuinely browser-only client subpaths, correcting the earlier over-broad 'browser-only; no Python runtime' note. - Correct the Google Chat file-uploads row: inbound attachment parsing is fully implemented (_create_attachment), so the gap is outbound file delivery only (post_message still logs 'not yet supported').
…PR 4/4) (#146) * chore(release): cut 0.4.30 — Teams SDK migration + 4.30.0 parity (#93 PR 4/4) Final PR of the Teams SDK migration (issue #93). PRs 1-3 (inbound+auth, outbound, native streaming) are merged; this cuts the 0.4.30 release. - Version bump: pyproject 0.4.29 -> 0.4.30; UPSTREAM_PARITY "4.29.0" -> "4.30.0". - Fidelity re-pin chat@4.29.0 -> chat@4.30.0 in lint.yml + verify_test_fidelity.py (docstring, default parity fallback, clone hint). packages/chat/src is byte-for-byte identical between the two tags, so zero new test ports: strict fidelity stays 100% (732/732, 0 missing) against chat@4.30.0. Baseline regenerated (ts_parity -> chat@4.30.0; the recorded total_ts_tests literal 731 -> 732 corrects a stale count from the merged adapter waves, not the re-pin — the count is identical against both tags). - Docs: project-instructions version map + fidelity pin; README status line; CHANGELOG 0.4.30 entry (Twilio adapter, Telegram streaming, Slack subpaths, WhatsApp/Slack/gchat fixes, Teams #93 PRs 1-4); UPSTREAM_SYNC.md parity header + the Teams deferral row flipped to delivered. - Version-label normalization: malformed `adapter-teams@chat@4.30.0` and loose `adapter-teams@4.30.0` -> `@chat-adapter/teams@4.30.0` in adapter.py (5), bridge.py (1), UPSTREAM_SYNC.md (4). Comment/doc-only. Does NOT tag/publish — the release is a separate maintainer-gated step (live Teams 429 streaming check + PyPI authorization). * docs(release): clarify 0.4.30 core-parity wording + finish label normalization Review fast-follow for PR 4. The CHANGELOG now states the mapped core is content-identical *between the chat@4.29.0 and chat@4.30.0 upstream tags* (verified: thread.ts/types.ts/thread.test.ts and the full packages/chat/src tree are byte-identical) — the prior 'unchanged from 4.29.0' phrasing was accurate but misread as a claim about our code. Also sweeps the two remaining old-style `adapter-teams@chat@4.30.0` labels in the Teams test docstrings to the canonical `@chat-adapter/teams@4.30.0` npm tag, completing the normalization the PR's scope called for. No logic change. * docs(release): document 0.4.30 parity-audit wave + remaining exceptions CHANGELOG: add 'Pre-existing parity gaps closed (4.30 audit)' subsection covering the 7 ported fixes (PRs #147-#150) and a documented-exceptions note (Linear agent-sessions #151 deferred to 4.31 / #152; adapter-web and the GitHub/Linear native-client + message.subject halves stay Known Non-Parity). UPSTREAM_SYNC: - file-mapping table: add state-ioredis -> redis.py (IoRedisStateAdapter) and state-pg -> postgres.py rows (both ported + tested, were missing). - Known Non-Parity (platform gaps): add Linear agent-sessions row (#151) and an adapter-web row that splits the portable server-side WebAdapter (deferred) from the genuinely browser-only client subpaths, correcting the earlier over-broad 'browser-only; no Python runtime' note. - Correct the Google Chat file-uploads row: inbound attachment parsing is fully implemented (_create_attachment), so the gap is outbound file delivery only (post_message still logs 'not yet supported'). * docs(release): correct phantom-feature framing before PyPI cut CHANGELOG: - Drop false gchat file-delivery bullet (PR #112 never merged; upstream itself does not implement gchat outbound file delivery) - Fix Slack DM block-action citation to merged PR #137 (supersedes #133) - Re-point 0.4.27.1 vehicle from unmerged #120 to tag v0.4.27.1 + #117 - Drop numberless Twilio 'scaffolding PR' (only #142 exists) UPSTREAM_SYNC: - Reframe Google Chat outbound file delivery as parity (upstream also logs 'not yet supported' + media.upload TODO, index.ts:1282-1289) - Correct Teams cert-auth row: TS is not 'Supported' — config throws at startup in both SDKs (types.ts:31, config.ts:13); parity, issue #58 - Add Messenger get_user row: upstream has no getUser; raising stub is parity, Graph-API impl tracked as #132 * docs(changelog): clarify 0.4.30 audit-gap count (8 closed + 1 deferred = 9)
patrick-chinchill
deleted the
fix/adapter-api-url-and-github-installation
branch
1 participant
Part of the 0.4.30 pre-ship gap-port wave (apiurl-github group). Pre-existing parity gaps the upstream-parity audit found — all ported faithfully from upstream chat@4.30.0. Not regressions.
(A) api_url custom-endpoint config — upstream 4.27.0 (6b17c60)
Upstream added an
apiUrlconfig (+<ADAPTER>_API_URLenv fallback) to every high-level adapter, routing a custom base URL into the underlying API client (proxies, API mocks, Enterprise / self-host endpoints). Four adapters were missing it:api_urlSLACK_API_URLbase_urlon BOTH the asyncAsyncWebClientcache and the syncweb_clientescape hatchapi_urlDISCORD_API_URL_discord_fetch(single HTTP funnel)api_urlGITHUB_API_URL_github_api_request+ installation-token exchange URLapi_urlLINEAR_API_URL_graphql_queryGraphQL endpointFor each: config wins over env, env over default, default unchanged when unset.
is not Nonehonors an explicit empty string (CLAUDE.md truthiness-trap hazard). Slack omitsbase_urlentirely when unset (slack_sdk rejectsbase_url=Noneand keeps its built-in default).(B) GitHub public
get_installation_id— index.ts:458-480Added the public
get_installation_id(thread | str)returning the fixed installation id (single-tenant app mode), the cached repo installation (multi-tenant), orNone(PAT mode); raisesValidationErrorwhen multi-tenant and uninitialized. The private_get_installation_id(owner, repo)helper is retained (delegated to in multi-tenant mode).Tests
tests/test_adapter_api_url_config.py— 31 tests:base_urlis passed when unset, plus an end-to-end check against the real slack_sdk normalizer).get_installation_id: all six auth-mode branches (PAT→None, single-tenant→fixed, multi-tenant cached→id, Thread-object input, multi-tenant uncached→None, multi-tenant uninitialized→raises).The usage-site assertions were verified to fail against the pre-fix sources (reverting each plumb made exactly the 8 custom-url usage tests fail).
Gauntlet (from worktree)
ruff check— cleanruff format --check— cleanaudit_test_quality.py— 0 hard failurespyrefly check— 0 errorspytest— 4804 passed, 3 skipped (baseline 4773; +31 from the new file)Notes
SocketModeClient(WebSocket event transport) is intentionally left unrouted — upstream'sslackApiUrlonly configures the Web API clients, not the socket connection.api_urlstrips a trailing slash sof"{base}{path}"joins cleanly with leading-slash paths (Enterprise endpoints often carry one). Linear's OAuth token endpoint (LINEAR_TOKEN_URL) is intentionally not overridden — upstream'sapiUrlonly targets the GraphQLapiUrl.